Sheffield | 26-ITP-January | Connor Parsons | Sprint 1 | Wireframe#1178
Sheffield | 26-ITP-January | Connor Parsons | Sprint 1 | Wireframe#1178connorpar wants to merge 16 commits intoCodeYourFuture:mainfrom
Conversation
…le-Onboarding into feature/wireframe
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
connorpar
added
the
Needs Review
cjyuan
added
Review in progress
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Page looks good and code is free of syntax errors. Good job.
- When a wireframe is provided, our implementation should closely reflect its appearance and layout to ensure consistency with design expectations. You're off to a solid start, but the main purpose of implementing a wireframe is to translate structure and functionality, not final visual design. Visual design is usually handled by UI/graphic designers after the wireframe phase. To better align with the wireframe, it would be better to
- Move the article title beneath the image and left justify it.
- Align the heights of the images in articles 2 and .
Wireframe/index.html
Outdated
| </article> | ||
| <article> |
There was a problem hiding this comment.
Indentation is slightly off.
It is a good practice to use an code formatting tool such as VSCode's "Format Document" to keep our code consistently formatted before we commit the changes.
There was a problem hiding this comment.
Ok thanks that's a good feature to know about.
Wireframe/style.css
Outdated
| } | ||
| h2 { | ||
| text-align: center; | ||
| font-size: 30px; |
There was a problem hiding this comment.
Using relative units for font size (e. g., em, rem, vw, %) is often better than fixed px because they make our design more accessible, flexible, and responsive.
Wireframe/style.css
Outdated
| #p_readme { | ||
| text-align: left; | ||
| font-size: 22px; | ||
| } | ||
| #p_description { | ||
| text-align: center; | ||
| font-size: 26px; | ||
| } |
There was a problem hiding this comment.
p_readme-- This id name would become "outdated" if the first article content changes.p_description-- This id name does not quite describe which paragraph it is identifying.
Can you think of more appropriate names that are more descriptive and are independent of article content?
Alternatively, you can use complex selectors to select the specific <p> element on the page without using id's.
cjyuan
added
Reviewed
|
I have now adjusted the webpage to better match the wireframe template by having the images at the top of each article section and extending the footer across the whole length of the viewport.
|
connorpar
added
the
Needs Review
|
Changes look good. Well done. |
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist